Skip to content
This repository was archived by the owner on Feb 4, 2022. It is now read-only.

fix(pool): don't try to reconnect if initial connection fails #332

Closed
wants to merge 1 commit into from

Conversation

vkarpov15
Copy link
Member

@vkarpov15 vkarpov15 commented Jul 22, 2018

Automattic/mongoose#6670 seems to be another instance of the issue I mentioned in #275: if initial connection fails, the driver will keep trying to reconnect in the background. So users that retry MongoClient.connect() when initial connection fails get a bunch of zombie database connections (one might call them "dead pools" :) ) if one of their reconnect tries succeeds. For example, try running this code against a standalone server that isn't running, and then start it after 10 seconds, you'll see 11 connections rather than 1.

var mongodb = require('mongodb').MongoClient;

var url = 'mongodb://localhost:27017/edata';
var options = {
        useNewUrlParser: true,
        autoReconnect: true,
        poolSize: 3,
        reconnectTries: Number.MAX_VALUE,
        reconnectInterval: 1000,
        socketTimeoutMS: 5000
}
var conectDate = new Date();
var count = 0;


// Connect to the db
var connectWithRetry = function () {
        mongodb.connect(url, options, function (err, db) {

                
                if (!err) {
                        console.log("We are connected");
                        db.on('close', function () {
                                console.log('Close + ' + count);

                        })
                        db.on('reconnect', function () {
                                console.log('Reconnect')
                        })
                }
                else {
                        setTimeout(connectWithRetry, 1000);
                        count++;
                        console.log(conectDate + ' - Database not connected - ' + count);
                }
        });
}

connectWithRetry();

Just wanted to put this issue out there and suggest a potential fix. We need a consistent explanation for how to handle initial connection failures: either the driver will keep trying to reconnect, or the user should try to reconnect on their own. Right now we generally recommend the latter, but the code does a little bit of both the latter and the former.

@vkarpov15 vkarpov15 requested a review from mbroadst July 22, 2018 02:46
@kereslas
Copy link

Probably same issue from #292.

Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Val. We'll be completely rewriting the pool logic in the next quarter, so we will definitely keep this fix in mind. I'll take a look shortly into why so many tests are failing here.

/cc @daprahamian

@daprahamian
Copy link
Contributor

@vkarpov15 I'm not sure that this is the right solution. Our API allows for the pool to attempt to reconnect multiple times before giving up. I think the main issue here is that we flush out all active work items and error them out before we even attempt reconnecting. See here.

I'm thinking that a fix for this will probably involve making the pool attempt to reconnect multiple times, and only once it fails should it flush out all of its callbacks. What do you think?

CC @mbroadst

@daprahamian
Copy link
Contributor

Actually, just dove into this: the problem is not the pool trying to reconnect, but the fact that we never destroy the pool when a mongo client fails to do an initial connect.

I will work on a fix for native and get that up ASAP

@daprahamian
Copy link
Contributor

@vkarpov15 I have PRs up at #335 and mongodb/node-mongodb-native#1797 to fix this.

@vkarpov15
Copy link
Member Author

Excellent, thanks @daprahamian . The fact that we still continue to reconnect even after initial connection fails is exactly the problem, thanks for fixing that. Any reason to keep this PR open or can I close it?

@daprahamian
Copy link
Contributor

@vkarpov15 we can close this.

@vkarpov15
Copy link
Member Author

Done

@vkarpov15 vkarpov15 closed this Aug 1, 2018
@vkarpov15 vkarpov15 deleted the vkarpov15-patch-1 branch August 1, 2018 17:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants